Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

relativize path for scalac classpath entry #8011

Merged
merged 1 commit into from Jul 4, 2019

Conversation

ity
Copy link
Contributor

@ity ity commented Jul 3, 2019

Problem

When using remote hermetic zinc (without native-image) we were encountering an error where a path was not found during remote execution. Digging into it, it was obvious that the path being sent wasnt being rewritten/relativized.

Solution

added relativization to all classpath entries for scalac, confirmed this fixes the issue locally.

@ity ity requested a review from stuhood July 3, 2019 23:47
Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ity ! This code is... hard to test, unfortunately. We need to get an end-to-end remoting test working, possibly via #7553, but until then: fine as is.

@illicitonion
Copy link
Contributor

Thanks @ity ! This code is... hard to test, unfortunately. We need to get an end-to-end remoting test working, possibly via #7553, but until then: fine as is.

If we're going to rely on spinning up scoot or something for integration testing here, we should think about doing so in a docker container, to avoid absolute paths happening to be present on the machine :)

@illicitonion illicitonion merged commit 2991f14 into pantsbuild:master Jul 4, 2019
@ity
Copy link
Contributor Author

ity commented Jul 8, 2019

Thanks @ity ! This code is... hard to test, unfortunately. We need to get an end-to-end remoting test working, possibly via #7553, but until then: fine as is.

If we're going to rely on spinning up scoot or something for integration testing here, we should think about doing so in a docker container, to avoid absolute paths happening to be present on the machine :)

thats sounds great to me!

@stuhood
Copy link
Sponsor Member

stuhood commented Jul 8, 2019

Thanks @ity ! This code is... hard to test, unfortunately. We need to get an end-to-end remoting test working, possibly via #7553, but until then: fine as is.

If we're going to rely on spinning up scoot or something for integration testing here, we should think about doing so in a docker container, to avoid absolute paths happening to be present on the machine :)

Mm, good point. Or maybe explicitly using RBE from an integration test will be reasonable sometime soon. cc @Eric-Arellano

@Eric-Arellano
Copy link
Contributor

I don’t think remoting an integration test will happen soon, unfortunately, due to starting teaching soon. Unless we expect it to be trivial to remote integration tests after we remote unit tests, which I don’t anticipate.

Unit tests should be soon! It seems all we need is to finish this token generator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants